Implement roadmap_aug10: authorization, storage parity, Iceberg conformance and the guardrails that catch them - #1
Open
AlexMercedCoder wants to merge 18 commits into
Open
Implement roadmap_aug10: authorization, storage parity, Iceberg conformance and the guardrails that catch them#1AlexMercedCoder wants to merge 18 commits into
AlexMercedCoder wants to merge 18 commits into
Conversation
Addresses the critical clusters of roadmap_aug10.md: the API-layer
authorization bypasses (B0a-B0m), the Iceberg metadata conformance and
commit-path defects (B11-B16o), and the middleware/reliability items.
Authorization (pangolin_api)
B0a POST /api/v1/tokens took no session at all: any authenticated
principal could mint a Root JWT for any tenant. Now Root-only,
or TenantAdmin within its own tenant and never above its rank.
B0b Credential vending performed no authorization and hardcoded
read+write for a table it never looked up. Now resolves the
asset, requires Read, and vends write only when Write is held.
B0c rename_table had no permission check. Now Write on the source
and Create on the destination namespace, plus a 409 on collision.
B0d update_namespace_properties discarded its session and never
resolved the catalog. Now Write-scoped to the namespace.
B0e create_view/get_view had no checks; a view's SQL is its whole
definition. Now mirror create_table/load_table.
B0f perform_maintenance ran destructive jobs against a hardcoded
"default" catalog with no authz. Now uses the path catalog and
requires Delete.
B0g The Iceberg OAuth token endpoint checked `active` but not
expiry, so an expired service user could renew indefinitely.
B0h PANGOLIN_DEV_MODE waived the NO_AUTH public-bind guard - the
two flags are routinely set together. The guard is unconditional.
B0i PermissionScope::Tenant matched without comparing tenants, so a
grant in tenant A satisfied resources in tenant B.
B0j Logout revoked session.user_id, which no token carries as its
jti, so tokens survived logout. UserSession now carries the jti;
rotate_token revokes the rotated-out token too.
B0k /api/v1/oauth/exchange was not public, making the OAuth login
flow unreachable: the browser could never redeem its code.
B0l OAuth linked accounts by unverified email. Identity is now
(provider, subject); email linking needs a verified address and
an operator domain allowlist.
B0m expires_in_hours could panic token issuance before the checked
arithmetic ran. Clamped, plus a CatchPanicLayer.
B0o A malformed or absent jti skipped revocation entirely; the
API-key branch also returned before the public-path check.
Iceberg (pangolin_core, pangolin_api)
B11-B14 default-spec-id (was current-partition-spec-id),
last-partition-id, schema "type": "struct", and no more
explicit nulls on optional fields.
B13 metadata-log is appended on every commit and truncated to
write.metadata.previous-versions-max.
B15 A client sequence number can no longer jump the counter to
i64::MAX and overflow the next commit.
B16 A feature-branch commit no longer moves main.
B16a One shared parse_namespace across all handlers: a nested
namespace registered on create is now found on commit.
B16b last-updated-ms advances on every commit, not just snapshots.
B16c -1 resolves against what this commit added, not vec.last();
duplicate Add* ids are rejected.
B16d/g Metadata files are written before registration and reclaimed
on a lost CAS instead of orphaned.
B16e create_table returned the table directory as metadata-location.
B16f The hand-rolled schema parser dropped complex columns, forced
every field optional and widened int to long. Deserialized now.
B16h Namespace property removals were silently ignored.
B16i pageToken/pageSize and next-page-token on list responses.
B16j Iceberg handlers return the spec error envelope.
B16k Federated forwarding on create/delete namespace and the tree.
B16l The timeout layer sits outside the concurrency limiter, so
queued requests have a deadline.
B16m delete_warehouse deletes before invalidating, closing the
window where a racing read re-cached deleted credentials.
B16n PANGOLIN_SHUTDOWN_GRACE_SECS actually bounds the drain.
Storage
B17 One ns_key helper on the memory backend; multi-level
namespaces were undeletable there.
New CatalogStore::delete_file and replace_namespace_properties,
implemented across all four backends.
Also completes loadNamespaceMetadata and namespaceExists.
cargo test --workspace: 57 targets green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses the storage-layer cluster of roadmap_aug10.md (B1-B7, B17-B30) plus improvement #1, the parity harness that makes the whole class of finding testable rather than reviewable. Tenant isolation and data integrity B1 Mongo's get_audit_event discarded the tenant_id parameter, so any tenant holding an audit UUID could read another tenant's record. B2 Mongo revocation was a silent no-op: the write went through serde as a string under `id`, the check queried a BSON UUID under `token_id`. Nothing could ever match, so revoked JWTs - including after logout - stayed valid. Cleanup had the mirror type mismatch and deleted nothing, so the collection grew without bound. B3 SQLite delete_branch referenced a column that does not exist and was non-transactional, so the branch was committed away and its assets orphaned while the caller saw an error. B4 Postgres decoded a TEXT[] as String; `Row::get` panics on decode failure, so any search with a hit panicked the request. SQLite's sibling silently returned raw JSON as the namespace. B5 Mongo dropped the compare-and-swap entirely, so two concurrent Iceberg commits both "succeeded" and one snapshot was lost. B6 The memory by-id asset index was keyed on catalog name alone, so tenant A deleting `sales` broke get_asset_by_id for tenant B's. B7 All three persistent backends stored the Debug spelling of AssetType and parsed only two variants, defaulting the other 15 to IcebergTable - a DeltaTable round-tripped as an Iceberg table. B26 Memory skipped the CAS whenever the expectation was None, which is the create-path assertion, not "no check". B30 Memory delete_tenant had no cascade: warehouses (with credentials), catalogs, assets and tokens all outlived the tenant. Parity and determinism B17 One ns_key helper on memory; create/get keyed on "." while delete/update keyed on 0x1F, so nested namespaces were undeletable. B18 SQLite's foreign_keys pragma is per connection but was set on one arbitrary pooled connection, making ON DELETE CASCADE fire nondeterministically. Now set through the connect options. B19 SQLite get_metadata_location ignored the branch and returned an arbitrary row, so a dev read could return main's pointer. B20 SQLite left the metadata_location column stale, freezing Asset.location at creation time after every commit. B21 SQLite delete_catalog cascaded before checking existence, so deleting a nonexistent catalog destroyed matching children first. B22 SQLite persisted Debug action names and parsed snake_case, then swallowed the mismatch, misattributing nearly every audited action. B23 Mongo audit filters compared Debug names to snake_case documents and always matched zero rows; listings were unsorted and unbounded. B24 Postgres list_catalogs omitted catalog_type/federated_config from the SELECT and hardcoded Local, so federated catalogs looked local. B25 Memory merge_branch reused asset ids (repointing the by-id index at the copy) and never advanced the target head. B27 ORDER BY on every paginated query; memory sorts before slicing. B28 One definition of search: LIKE metacharacters escaped, and one tag semantic (ALL-match, empty list means no filter) across all four. B29 Memory audit returned oldest-first and unbounded without a filter. Two defects the new parity suite found on its first run * SqliteStore had no inherent revoke_token/is_token_revoked, so the trait delegations in sqlite/main.rs called themselves. Revoking a token on SQLite recursed until the stack was exhausted and aborted the process. * The SQLite audit_logs table still declared the original (actor, resource, details) shape while the code inserted the full AuditLogEntry, so every audit write failed with "no such column" and the backend kept no audit trail at all. Schema version bumped to 2. New CatalogStore methods delete_file and replace_namespace_properties are implemented across all four backends. cargo test --workspace: 57 targets green. Clippy budget lowered 36 -> 34. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses B8-B10 and B43-B46 of roadmap_aug10.md, plus improvement #3.
Deployment
B8 `docker compose up` could not start the API: the quick-start compose
file set no PANGOLIN_JWT_SECRET, and since 0.6.0 the server refuses to
start without one. The result was a crash-looping container with no
explanation. The `:?` form now fails `docker compose up` itself with a
message telling you what to set.
B9 Both compose files set PANGOLIN_STORE_TYPE. The server reads
PANGOLIN_STORAGE_TYPE, so the variable did nothing - and anyone editing
it to `postgres` silently stayed on the in-memory backend and lost
their data on every restart.
B10 docker-compose.release.yml pinned alexmerced/pangolin-api:0.2.0, four
releases behind, and ran scripts/test_release_v0.2.0.py, which does not
exist. The "release verification" file verified nothing. Now
parameterised by PANGOLIN_VERSION and pointed at a real script.
Docs
B43 docs/environment-variables.md documented PANGOLIN_HOST, PANGOLIN_PORT
and PANGOLIN_STORE_TYPE - none of which any code reads - and omitted 34
that it does. Rewritten from the source, and the second, separately
drifted copy under getting-started/ is now a redirect.
The fix is not the rewrite; it is scripts/check_env_var_docs.sh, which
re-derives the set from the code and fails CI on either kind of drift.
A `<!-- not-a-variable -->` marker lets the page still warn readers off
names that do not exist.
Hygiene
B44 A live PyPI API token sat in plaintext in the repo-root .env. It was
never tracked by git, but .env is read by docker compose - so a publish
credential was being handed to containers with no use for it, and it
was one .gitignore edit away from leaking. Removed, with a pointer to
the keyring / CI-secret flow PUBLISHING.md already describes.
** The token must be rotated at pypi.org: it existed on disk in
plaintext and has to be treated as exposed. **
B45 git rm --cached on ~260 KB of tracked debug output, deleted the two
.bak store monoliths (~4k lines of divergent dead query copies that
any grep of the store layer would hit), and added the ignore patterns
that were missing when those files were first committed.
B46 Pruned 18 unused runtime dependencies (all @smui/*, marked,
smui-theme) from the UI - zero imports in src/, pure image bloat.
Added the missing @vitest/coverage-v8 so `npm run test:coverage` runs,
added a test:e2e script for the Playwright specs that had none, and
pointed Playwright at 5173 (where vite dev actually serves) with
webServer enabled, so the specs connect to something.
CI (improvement #3)
A `config drift` job validates every compose file, asserts the quick start
still fails loudly without a signing secret, rejects any PANGOLIN_* name in
a compose file that no code reads, and runs the env-var doc check. All three
of B8/B9/B10 are the same failure - config nothing validates - so the job
targets the class rather than the instances.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses B31-B37 of roadmap_aug10.md, plus improvement #10. Most of these are
the same defect: the UI and the server had drifted apart with nothing checking.
Base URL and transport
B31 Three different names for the API base URL coexisted - the client read
PUBLIC_API_URL, .env.example declared VITE_API_URL, the compose files
passed VITE_API_URL, and the login page read a fourth spelling off
import.meta.env. None agreed, so the client's fallback always won and
every deployed build called `http://localhost:8080` - the *visitor's*
machine. Standardized on PUBLIC_API_URL (SvelteKit's dynamic public env
requires the prefix), defaulting to same-origin in production.
B32 ~13 raw `fetch('/api/v1/...')` calls only resolved under the dev proxy.
With adapter-node they hit the SvelteKit server, which has no /api/v1
routes, and 404'd in production - and they skipped X-Pangolin-Tenant, so
a root user acted on the wrong tenant. All routed through apiClient.
That includes the NO_AUTH probe in the auth store, which always
concluded "auth enabled" because its relative URL 404'd.
B34 Nothing in src/ handled a 401, so an expired JWT left the user in a
permanently broken "authenticated" session. The client now reports 401
to a handler the root layout registers, which ends the session and
redirects. Logout also revokes the token server-side - it previously
only cleared localStorage, leaving a working credential behind.
Endpoints the UI called that did not exist (B33)
Added `DELETE /api/v1/branches/{name}?catalog=...`: the UI's branch-delete
control had always 404'd because only GET was registered.
Added `GET /api/v1/oauth/providers` (public): the login page hardcoded four
provider buttons because this 404'd, so buttons for unconfigured providers
went nowhere. Now only configured providers render.
Pointed the permissions client at `GET /api/v1/permissions?user=<uuid>`,
which exists, instead of `/users/{id}/permissions`, which never did - the
permissions page and EditPermissionsDialog showed an empty list forever.
Removed `initiateOAuth`, which called another route that does not exist.
Other
B35 Tag-filtered search was broken end to end: the UI sent repeated `tags`
params, and `serde_urlencoded` cannot deserialize repeated keys into a
Vec, so it 400'd. Now one comma-separated value on both sides.
B36 DataTable's only actions-slot outlet sat inside an
`{#if searchable && !serverSide}` guard; the catalogs page passes
neither, so its "New Catalog" control never rendered and there was no
way to create a catalog from that page. The page also refetched the
same unpaginated list and never set hasNextPage. Both fixed, plus the
HTML comments in attribute position that Svelte parsed as boolean props.
B37 The root layout used `tenantStore` with no import (a latent
ReferenceError), the handler was wired to no element, and the tenant
loader was commented out - so root users could not switch tenants and
X-Pangolin-Tenant was never set. Imported, re-enabled, and the selector
is back in the header.
B46 test plumbing, and what it was hiding
vitest.config.ts overrode `import.meta.env` wholesale, which broke
SvelteKit's virtual env module - so every suite importing the API client
failed to *load*. That is why client.test.ts's mocks could lack `.text()`
without anyone noticing. With the override gone and the mocks completed,
UI suites passing went 7 -> 11 and tests passing 37 -> 86.
40 tests still fail. They are pre-existing and outside this roadmap's scope:
Svelte 5 dropped `$on` on component instances, and a number of assertions
were written against an older UI. They were previously invisible because the
suites never loaded; making them visible is the point.
Improvement #10: deleted `src/lib/stores.ts`, `app.scss`, and the
unauthenticated `routes/api/docs/[...path]` file-read route (no callers, a
blacklist-based traversal guard), and stripped 27 console.log calls - one of
which logged a bearer-token prefix to the browser console.
cargo test --workspace: 57 targets green. `npm run build` succeeds.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses B38-B41, B_cli1-B_cli8 and B_sdk1-B_sdk5 of roadmap_aug10.md, plus
the `deny_unknown_fields` leg of improvement #0.
The systemic fix first
Every finding in this cluster has the same shape: a client sends a field the
server does not have, serde ignores it, and the request succeeds having
quietly done something other than what was asked. `deny_unknown_fields` on
all 34 server request structs turns that into a 422 that names the field.
It immediately caught five payloads in the project's own tests. The worst:
full_system_test.rs created a catalog with kebab-case `warehouse-name` and
`storage-location`, both dropped - so it asserted 201 for a catalog with no
warehouse and no storage location, and the warehouse existence check never
ran. That test now creates the warehouse it always claimed to reference.
Python SDK
B38 __version__ was hardcoded "0.1.0" against a 0.6.0 package; now read
from the installed distribution.
B39 No timeouts anywhere - a hung server blocked the caller forever. Every
request is now bounded, and the client reuses a Session.
B40 requires-python ">=3.8" was unsatisfiable with pydantic>=2 and
pyiceberg; bumped to >=3.9 with matching classifiers.
B41 Importing anything from pypangolin pulled in the whole Iceberg stack,
so `pytest tests/` could not even collect. pyiceberg is an extra and
catalog.py imports it lazily; tests now collect and pass (5/5).
B_sdk1 PermissionScope's kebab-case aliases never matched - the server emits
kebab-case *variants* with snake_case *fields*, which I verified against
its actual output rather than inferring. Every scope deserialized empty.
Role.permissions was typed List[Permission] where the server returns
PermissionGrant, so any role with grants raised ValidationError.
B_sdk2 delete(asset_id, key) deleted *all* metadata; request_access sent
`motivation` where the server reads `reason`; federated create dropped
uri/warehouse/credential; token generate sent three ignored fields so
every token was silently 24h; rebase omitted the required `name`.
B_sdk3 Four CLI commands raised TypeError or AttributeError on every call,
masked by blanket handlers that exited 0.
B_sdk4 profiles.yaml (holding the JWT) was written at the umask - now 0600
under a 0700 directory, with the mode set before the secret is written.
get-token no longer echoes the token into the terminal, and connection
assets no longer store the encryption key beside its own ciphertext.
B_sdk5 Added the console-script entry point the docs promised, py.typed,
pytest pythonpath, `raise ... from e`, and the ConfigDict migration.
Rust CLIs
B_cli1 create-catalog sent `warehouse`/`type`; the server takes
`warehouse_name`/`catalog_type`. Both were dropped, so the required
--warehouse flag was discarded, the warehouse existence check was
skipped, and the CLI printed success.
B_cli2 All six merge commands targeted /api/v1/merges/..., a prefix that has
never existed.
B_cli3 Four federated commands used /api/v1/catalogs/{name}/... instead of
/api/v1/federated-catalogs/...; create-federated-catalog made a *Local*
catalog; list-federated-catalogs filtered on a key that is not in the
response, so it always printed empty.
B_cli4 Both revocation commands 404'd (/api/v1/tokens/revoke*, real routes
are under /api/v1/auth/).
B_cli5 delete-user passed a username to a Path<Uuid> handler (400 every
time; it now resolves the id first); revoke-permission used a filter
DELETE that does not exist; request-access posted to a GET-only route;
three `.unwrap()`s on server JSON panicked the CLI.
B_cli6 update-user --username was silently dropped (the server cannot rename
a user); unhandled commands printed a note and exited 0.
B_cli7 ConfigManager::new().unwrap() panicked with no $HOME; the config file
holding the token was written 0644; reqwest had no timeout.
B_cli8 merge-branch sent source/target instead of source_branch/target_branch;
request-access was a no-op returning success; search was a placeholder
telling users a working feature did not exist; get-token sent a null
tenant; generate-code printed the live JWT into copy-paste output.
Also closed a metadata-delete authorization gap found on the way: the handler's
entire authorization was the comment `// Check permission logic`, so any tenant
member could delete any asset's description, tags and discoverable flag.
Every CLI command now exits non-zero on failure (21 sites). That is what made
this cluster survivable: the errors were printed and the exit code was 0, so no
script and no CI job could tell a working command from a broken one.
cargo test --workspace: 57 targets green. pytest: 5 passed. Clippy at budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes roadmap improvements #0, #1 and #2, and fixes B42 plus one more handler with no authorization check. The permission matrix (improvement #0) Every finding in the B0a-B0m cluster was invisible to CI. The code compiled, was formatted, was lint-clean, and the tests passed - because nothing asserted *who is allowed to call what*. A handler that forgets `check_permission` looks identical, to every check that existed, to one that calls it. `POST /api/v1/tokens` minting a Root JWT for any authenticated caller survived a full security release that way. `authz_matrix_tests` drives each sensitive route as Root, the owning tenant's admin, an ungranted tenant user, and a foreign tenant's admin, asserting the expected 200/403 for each. It covers token minting and rank limits (B0a), the token-lifetime panic (B0m), view endpoints (B0e), maintenance (B0f), credential vending (B0b), cross-tenant catalog access (B0i), logout actually revoking (B0j), the OAuth exchange endpoint staying public (B0k), unknown request fields being refused, and anonymous callers being turned away. Verified by reintroducing B0a behind a one-line edit: the matrix fails with `200 where 403 expected`. A test that cannot fail is not a guardrail. Deliberately end-to-end through the real router - a unit test of `check_permission` proves the function works, not that the handler calls it, and "the handler does not call it" is the whole bug class. CI (improvements #1, #2, #3) * `guardrails` runs the permission matrix and the cross-backend parity suite as their own job, so a failure reads as "authorization or backend parity regressed" rather than as an anonymous test failure. * `sdk` runs pytest and ruff, asserts the package imports *without* the Iceberg extra (B41), and asserts `__version__` matches package metadata (B38). * `ui` builds, and greps for the two patterns a type checker cannot express: a raw `fetch('/api/v1/...')` reappearing (B32) and any resurrection of `VITE_API_URL` (B31). * `config-drift` (previous commit) validates the compose files and the environment-variable reference. Two more authorization gaps, same class as B0c-B0f * `rebase_branch`: the entire authorization was `// TODO: Granular permissions? For now assume Write on Catalog`. Any tenant member could rebase any branch, which rewrites its contents. * `delete_business_metadata`: the authorization was the comment `// Check permission logic`. Any tenant member could delete any asset's description, tags and `discoverable` flag - the last of which governs who can see the asset at all. B42: `list_catalogs` paginated in the store and filtered by permission afterwards, so a TenantUser got variable-size pages *including empty pages while more authorized data existed* - and a client that stops on an empty page, which is the normal idiom, silently missed data. Filtering now happens before the page is cut. Also brought ruff to zero on the SDK (unused imports, discarded bindings), with E701 turned off explicitly and with a reason rather than by omission. cargo test --workspace: 57 targets green. pytest: 5 passed. ruff: clean. `npm run build`: succeeds. Clippy at its 34 budget, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every finding in roadmap_aug10.md, grouped by what an operator needs to act on rather than by the audit's own numbering - with the five findings exploitable by any authenticated principal called out first, and the PyPI token flagged for rotation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nly a
live database could reveal
Four items requested after the roadmap work: a real SQLite migration, running
the suites against live Postgres and MongoDB, a security advisory for 0.6.0,
and the version bump with migration notes.
1. The SQLite audit_logs fix did not reach existing databases
The previous commit corrected `sqlite_schema.sql` and bumped
SQLITE_SCHEMA_VERSION. That is not a migration: the schema file is written
entirely with `CREATE TABLE IF NOT EXISTS`, which does nothing when the table
already exists. Fresh installs got the corrected columns; every upgraded
database kept the broken ones, now with a version number claiming otherwise -
worse than no version at all.
There is now a real v1 -> v2 step, keyed off `PRAGMA table_info` rather than the
recorded version (some databases predate the version table, and a version only
says what a previous run claimed). The old table is preserved as
`audit_logs_pre_v2` rather than dropped or force-fitted: its shape has no
`resource_type`, which is NOT NULL and parses as an enum, and a fabricated entry
in an audit log is worse than an absent one.
`tests/sqlite_migration_tests.rs` builds a genuine pre-v2 database and asserts
the upgrade works. Verified to fail without the migration - three of its four
cases, exactly the upgrade paths.
2. Running against live Postgres and MongoDB
The parity suite was written against memory and SQLite, the two backends CI
could run without a service container. Pointed at a live Postgres and MongoDB
for the first time, it failed on both. None are regressions.
* **Postgres: asset search was broken outright.** No migration ever created
`business_metadata`, while `search_assets` joins it - every search failed
with `relation "business_metadata" does not exist`, a hard SQL error rather
than an empty result. The three CRUD methods were unimplemented, so the
trait's "not supported" default answered them. Added both.
* **Mongo: role assignments were unreadable.** `bson::to_document` writes a
`Uuid` as a string while the deserializer expects Binary, so `assign_role`
wrote documents `get_user_roles` could never match and that could not be
deserialized at all. Every role-derived permission silently vanished: a
user holding an admin role was authorized as though they held none. The
same asymmetry caused B1 and B2 in two other collections; one helper now
covers all of them.
* **Mongo: `get_metadata_location` had no fallback** to the asset's own
`location`, unlike the other three backends, so the read path and the
commit CAS disagreed about what "current" meant.
* **Mongo: the "no transaction support" fallback was unreachable.**
`start_transaction` is a local call in the Rust driver and cannot fail for
want of a replica set; the error arrives on the first operation inside the
transaction and was propagated rather than caught. `delete_catalog` failed
outright on standalone mongod instead of degrading as its comment promised.
Also made `test_postgres_access_requests` re-runnable - it used a fixed
username and only passed against a freshly created database.
CI now runs the parity suite against Postgres, MongoDB and MinIO service
containers, and **fails if any backend was skipped**. A skipped backend passing
silently is precisely how two of them went untested through a security release.
3. Security advisory
SECURITY.md now carries the 0.7.0 advisory: ten issues exploitable by any
authenticated principal, the two availability defects, the audit-trail gaps,
and the data-loss class - with the rotation steps each one implies. 0.6.x is
marked unsupported.
4. Version and migration notes
`scripts/bump_version.sh` sets one version across all five artifacts *and* the
inter-crate path requirements (missing those breaks the build outright, as this
commit found out), with a `--check` mode wired into CI. Everything is at 0.7.0.
`docs/upgrading/0.6-to-0.7.md` covers the breaking changes: unknown request
fields now rejected, branch commits no longer moving main, tokens without a
jti, OAuth email linking, the renamed compose and UI variables, and the four
changed SDK signatures - each with what it was silently doing before.
Verified with live Postgres, MongoDB and MinIO: 58 test targets, exit 0, zero
failures. Clippy at its 34 budget, fmt clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
✅ Deploy Preview for lustrous-pithivier-968b22 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…osed Everything verified on MongoDB so far ran against a *standalone*, which supports neither transactions nor retryable writes - so only the degraded branches ran. That is the same gap that produced the last round of findings, one layer in: the fallback in `delete_catalog` was broken for as long as the code existed precisely because nothing had ever run the branch it falls back *from*. A single-node replica set is enough to tell the two apart - a set of one has transactions and retryable writes - so there is now `docker-compose.mongo-rs.yml` for local use (on port 27018, so it can run alongside the standalone) and a `mongo-replica-set` CI job. The job asserts a primary was elected *and* probes that a transaction actually commits, because a job that silently re-tests the standalone paths is worse than no job. Running it found another defect, in the fallback this time: `delete_catalog_unsafe` deleted every matching tag, branch, asset and namespace and only then discovered the catalog did not exist - returning "not found" to a caller with every reason to believe nothing had happened. That is B21, fixed for SQLite during the roadmap work; the same shape survived here because this branch only runs without transactions and had never executed. The parity test for B21 also needed sharpening. It named a catalog with no children at all, which passes whatever the ordering is - it constrained nothing. It now creates an orphaned child (a namespace under a catalog name with no catalog row) and asserts it survives a failed delete. Verified to fail against the unfixed Mongo path. That change surfaced a genuine backend asymmetry worth recording: Postgres is the only backend where the orphaned state is *unreachable*, because it carries foreign keys from namespaces to catalogs and from assets to namespaces. The test asserts the ordering where orphans are possible and says so explicitly where they are not, rather than skipping quietly - "Postgres refused to create the fixture" and "Postgres passed the assertion" are very different facts. All four backends green. Clippy at budget, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The string/Binary mismatch had been fixed four times, in four collections,
each time as its own bug: audit events, token revocation, role assignments,
permission records. Fixing them one at a time was treating symptoms. This
audits all 21 collections at once, with a round-trip test per entity rather
than per feature, and finds four more.
There are three ways this codebase converts a Uuid to BSON and they all
differ. `to_bson_uuid` gives Binary with the generic subtype; `doc! { "k": id }`
gives Binary with the UUID subtype; `bson::to_document` gives a string. Reads
disagree too: a typed Collection<T> demands binary, `bson::from_bson` demands a
string. A write and a read chosen independently agree only by luck, and the
failure is silent — the filter matches nothing and the caller gets an empty
result indistinguishable from an empty collection.
What that was costing:
* Every service-user method was a no-op. Writes let Mongo generate an ObjectId
while the by-id methods filtered on `_id`; the listing and the API-key lookup
used snake_case names for a kebab-case struct. API-key authentication could
never resolve a service user on MongoDB. It fails closed, so this was an
outage rather than a bypass.
* Business metadata could be written but never read: only asset-id was
rewritten as Binary, so the first of the other three UUID fields aborted the
read. Writing metadata made it permanently unreadable.
* Listing active tokens failed outright — timestamps written as BSON DateTime,
read by a chrono deserializer that only accepts RFC3339. The created_at arm
swallowed the same error and substituted now().
* A branch with a head commit could not be read. Only branches that had never
been committed to worked, which is why every existing test passed.
`from_bson_uuid` now accepts all three encodings so records already written by
any of them still load; writes go through `to_bson_uuid` alone. The new suite
covers every collection and runs against both MongoDB topologies in CI.
Two test-environment defects surfaced while verifying this. The db-test compose
file had no object store at all, so the compliance tests fell through to the EC2
metadata endpoint and failed with a credentials error naming nothing relevant —
CI had MinIO and the documented local workflow did not. And the image CI pulled,
bitnami/minio:latest, has been withdrawn from Docker Hub: that job could not have
been passing. Both now use minio/minio with explicit bucket creation.
Verified against live MongoDB in both topologies and live PostgreSQL: full
pangolin_store suite green, clippy at budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The UI job in CI built the app and never ran its tests. The suite had drifted to 40 failures across 14 files, and most of them could not have passed: the global test setup replaces the API and store modules with stubs, so the unit tests *for those modules* were asserting against the stub rather than the code. Running them turned up real defects underneath. * A root user could not create another root user - the role option's value was `Root` and the server's `UserRole` is kebab-case. The same PascalCase leftovers showed a tenant admin an Edit control for root users, and left the role badge colours keyed on values the API never returns. * A warehouse created in the UI showed no bucket in the UI: the list page read `s3.bucket`, the create form writes `bucket`. Both are now accepted on read, as the server already does. That table also rendered Type twice, in place of the Region column its own template had a branch for. * `production` is not a branch type. The API knows `ingest` and `experimental`; the UI typed it as `'experimental' | 'production'` and keyed the green badge on `production`, so every branch rendered identically and `ingest` - the type carrying a distinct permission - had no representation at all. * A branch with no recorded parent was displayed as branching from `main`. * A local catalog could be created with neither a warehouse nor a storage location, leaving it nowhere to write its tables. * The role select on the user edit page had no accessible name, and its fallback value matched none of its options. * `logout()` could throw and skip the caller's redirect, stranding the user on a page they were no longer authenticated for. `DataTable` moves from `createEventDispatcher` to callback props, with its six consumers. That is the Svelte 5 idiom and it is what makes the component testable: `component.$on(...)` was removed in Svelte 5, so the row-click test had been sitting there as a stub asserting nothing. CI now runs `npm test` and carries a svelte-check budget on the same ratchet as the clippy one. svelte-check errors fell 166 -> 150, mostly by giving `StorageConfig` the index signature the server's free-form HashMap always implied. The app runs in Svelte 5 legacy mode - none of its 90 components use runes. That is supported and works; converting them is separate work and is not done here. 129 tests pass, build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…comments
`gh pr checks` on the last push: five jobs red. Everything in the previous two
commits was verified locally, which turns out not to be the same thing.
The important one: the workflow set `RUSTFLAGS: -D warnings` at the top level.
`clippy` and `test` override it with `RUSTFLAGS: ""`; the two jobs added for the
audit - `guardrails` and `mongo-replica-set` - did not, so they failed at
*compile* on the tree's pre-existing dead-code warnings. The permission matrix,
the cross-backend parity suite and every MongoDB suite they were added to run
had never once executed. A guard that cannot run is worse than no guard,
because it reads as protection. The top-level setting is removed rather than
overridden in two more places: it gated nothing, since every job that compiles
Rust opted out of it, and `clippy-ratchet` is the actual gate on warnings.
The rest:
* `config drift` failed on its own documentation. Its `grep -v 'not read'` was
meant to skip explanatory comments and matched neither of the two that exist
("was renamed", "which nothing reads"). Both guards now strip comment lines -
the same fix already applied to the UI guards in this file.
* That guard was also not recursing, so it had never seen the eight
`deployment_assets/**/docker-compose.yml` files still setting
`PANGOLIN_STORE_TYPE`, which the server does not read. B9 fixed the two
compose files at the repo root and never reached these. They work anyway -
each sets `DATABASE_URL` and the server infers the backend from its scheme -
so this was dead, misleading config rather than an outage. Renamed to
`PANGOLIN_STORAGE_TYPE`; all four values in use are accepted by `build_store`.
* `python sdk` failed on 316 ruff findings. `pyproject.toml` says the ruleset is
"pinned here rather than left to whatever version the runner installs" - but
the job ran `pip install ruff`, which took 0.16 and its wider default rules.
Pinned to the version the package was actually clean against.
`PANGOLIN_API_URL` is exempted from the drift guard with the reason recorded:
it is set on the `tests` service in docker-compose.release.yml, and nothing
reads it because the script that service runs, scripts/integration_test.py,
starts its own server with `cargo run` - which cannot work in a
python:3.11-slim container with only ./scripts mounted. B10 replaced a missing
script with one that cannot run there. That path is still broken and is not
fixed here.
Both guards and the compose files verified locally; ruff and pytest clean at
the pinned version.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo audit` reports 26 vulnerabilities against the current lockfile, which is
the one CI job still red. Several are not cosmetic for this service: two
high-severity certificate-validation bypasses in `aws-lc-sys`, name-constraint
and CRL-parsing defects in `rustls-webpki`. Pangolin vends cloud credentials
and talks to S3, Azure and GCS over TLS, so certificate validation is on its
critical path.
This is the subset that needs no policy decision: targeted `cargo update -p`
for each advisory with a compatible fix, leaving `rust-version = "1.92"` alone.
26 -> 17.
Cleared: bytes 1.11.0 -> 1.12.1, crossbeam-epoch 0.9.18 -> 0.9.20,
rustls-webpki 0.103.8 -> 0.103.13, hickory-proto, quinn-proto, time.
A blanket `cargo update` reaches 8 rather than 17, but pulls AWS SDK crates
requiring rustc 1.94.1 against a declared MSRV of 1.92. Raising the toolchain
floor breaks every consumer still on 1.92, which is a decision about this
project's users rather than a side effect of a dependency bump, so it is not
taken here.
What remains needs one of two deliberate choices, still open:
* an MSRV bump to 1.94, which unblocks the `aws-lc-sys` cluster and the AWS
SDK chain;
* an `.cargo/audit.toml` ignore list with per-advisory justification. Some
entries need this route regardless - `rsa`'s Marvin timing attack
(RUSTSEC-2023-0071) has no fixed version at all, and `quick-xml` is held
back by `azure_core` 0.20 and `object_store` 0.11.
Verified: 55 test targets, all ok, zero failures, against live PostgreSQL,
MongoDB and MinIO.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… never built Takes both halves of the dependency plan - bump what a bump fixes, justify in writing what nothing can - and turns up something larger on the way. ## The cloud-credential features have never compiled `cargo check -p pangolin_api --features cloud-credentials` fails. So do `aws-sts`, `azure-oauth` and `gcp-oauth` individually. This reproduces at the original `aws-sdk-sts = "=1.50.0"` pin, so it is not fallout from this change: they have never built, at any version. `cargo build` and `cargo test` run with default features and no job ever passed `--features`, so nothing noticed. For a catalog whose job includes vending scoped, time-limited cloud credentials, that is the feature set. The errors are the kind that only surface when a `cfg` block is never type-checked: five parameters bound as `_duration` / `_resource_path` / `_permissions` to silence unused warnings in the default build and then referenced without the underscore inside the feature block, across three signers; `anyhow!` used in gcp_signer.rs with only `anyhow::Result` imported; and `creds.expiration()` handed to `chrono::DateTime::parse_from_rfc3339`, though it returns an `aws_smithy_types::DateTime` and never was text. The operational consequence, verified in the code rather than assumed: with the feature off the `#[cfg]` block is absent, so `generate_credentials` falls through to the static-credential branch and vends the long-lived warehouse key with `expires_at: None`. Any deployment that set `use_sts` and a `role_arn` believing it received a scoped session token did not. Recorded in SECURITY.md. The `=1.50.0` pin carried no comment, protected nothing, and blocked `aws-config` from the release that drops the second TLS stack. Relaxed to 1.109. ## Advisories: 26 -> 0 Upgrades cleared the ones that matter here: the `aws-lc-sys` cluster (two high-severity certificate-validation bypasses, a PKCS7 signature-validation bypass), `rustls-webpki` name-constraint and CRL-parsing defects, plus quinn-proto, hickory-proto, bytes, crossbeam-epoch and time. Those sit on the path this service uses to reach S3, Azure Blob Storage and GCS. Reaching them needed the MSRV at 1.94, raised from 1.92 deliberately. The remaining eight are exceptions in .cargo/audit.toml, each with the reason it cannot be upgraded and what the exposure actually is. They are specific advisory IDs, so a new advisory - including a new one against these same crates - still fails the job. `rsa` is the clearest case: never compiled at all, since `sqlx-mysql` is an optional dependency this workspace does not enable, and `cargo audit` scans Cargo.lock rather than the build graph. ## Two new guards `features` builds each optional feature, and the store's azure/gcp backends - the check whose absence let the above rot. `msrv` reads `rust-version` out of the manifest and builds with exactly that toolchain. Every other job runs `stable`, so the declared floor had never been tested; it was a promise to consumers that nothing kept. Dockerfile, README, CONTRIBUTING and the deployment guide are all moved to 1.94 with it. Clippy rose 34 -> 37 on the newer toolchain; cleared rather than ratcheted up, now 31, budget lowered to match. Verified: 59 test targets, 370 tests, zero failures, against live PostgreSQL, MongoDB and MinIO. All four optional features compile. fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The advisory exceptions landed in `pangolin/.cargo/audit.toml` and the job kept failing on the very advisories they cover. `rustsec/audit-check` invokes `cargo audit --file pangolin/Cargo.lock` from the repository root, and `cargo audit` reads `.cargo/audit.toml` relative to the current directory only - it does not walk up the tree. Reproduced locally: `cargo audit` from `pangolin/` reports zero, the identical lockfile audited from the root reports eight. Duplicating the config at the repository root would have turned CI green while leaving a developer running `cargo audit` in `pangolin/` - where the workspace actually is - looking at a different answer from the one CI computes. That is the same shape as the defects this branch exists to fix, so the job now runs the same command in the same directory a developer would, and there is one copy of the file. Everything else on the previous push passed, including the two jobs added with it: `minimum supported rust version`, and `optional features build` across all four features. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything needed before publishing 0.7.0. Each item here is a reason the previous four releases did not actually ship what they claimed. **The tag workflow has never succeeded.** `build-macos-intel` targets `macos-13`, and GitHub retired that image in December 2025. Both v0.6.0 runs sat queued for the full 24-hour limit waiting for a runner that no longer exists, while linux, macos-arm and windows finished in 8-15 minutes; `create-release` was skipped because it `needs:` all four. v0.4.0, v0.5.0 and v0.5.1 were cancelled the same way. That is why the repository has tags but no releases and no published binaries. Moved to `macos-15-intel`, GitHub's documented replacement for x86_64, and bumped the release action off an unmaintained v1. **The Docker build script was pinned to 0.3.0** while the project shipped 0.4.0, 0.5.0 and 0.5.1 - so running it would have overwritten the 0.3.0 tags and published nothing under the current version. It now reads the version from the workspace manifest, so it cannot drift again, and refuses to overwrite a tag that is already published unless ALLOW_OVERWRITE=1. `latest` and the version tag are pushed together. **Release verification never verified anything.** The `tests` service in docker-compose.release.yml pointed first at a script that did not exist (B10), then at `integration_test.py` - which builds and starts its own server with `cargo run`, and so cannot run in the `python:3.11-slim` container it is given, with only ./scripts mounted and no Rust toolchain. Replaced with `release_smoke_test.py`, which talks to the running container over HTTP: waits on `/health/ready` (which probes the store, unlike `/health`), asserts `/health/live` and that `/metrics` exports at least one series, checks the Iceberg `/v1/config` shape, and - in no-auth mode - round-trips a catalog. The compose default moves 0.6.0 -> 0.7.0, and the service now mirrors the server's own PANGOLIN_NO_AUTH instead of a TEST_MODE nothing read. **The advisory named the wrong affected range.** SECURITY.md said "0.6.0 is affected"; the published image `alexmerced/pangolin-api` was last pushed at 0.5.1 and no 0.6.0 image ever existed, so most deployments are on 0.5.1 - which carries everything in the advisory plus everything 0.6.0 fixed. Corrected to `< 0.7.0`, with a note telling readers to check what they are actually running. CHANGELOG's Unreleased section becomes 0.7.0, dated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The release smoke test's first real run, against the built 0.7.0 image, found
this. It would have shipped.
`shutdown_grace` is meant to bound the *drain* - how long shutdown may spend
finishing in-flight requests after SIGTERM. B16n implemented that as:
tokio::time::timeout(shutdown_grace, serve)
`serve` is the whole server, not the drain, so this bounded the lifetime of the
process. With the default 25-second grace, the server started, served happily,
and then exited 0 with "drain did not finish within the shutdown grace period"
having received no signal at all. Every container would have crash-looped, and
`PANGOLIN_SHUTDOWN_GRACE_SECS` was a countdown to death rather than a limit on
draining.
All 18 CI jobs passed with this present. So did the full workspace suite, twice,
against live databases. Nothing runs the binary for longer than 25 seconds.
The deadline now lives in `shutdown_signal`, armed only once a signal has
actually been seen, as a detached task so returning still lets axum begin its
drain. Verified directly: with a 5-second grace the server stayed up through 20
seconds of polling at health=200, then exited 2 seconds after SIGTERM with
"shutdown complete".
The `docker` job now runs the built image with a 5-second grace, waits 20
seconds, fails if it is no longer serving, and then confirms it still stops
promptly. That is the check whose absence let this through.
Two further defects in the release harness, both found the same way:
* It inherited the developer's `.env`. Compose auto-loads it, so a local
PANGOLIN_ROOT_USER / PANGOLIN_ROOT_PASSWORD fed the verification - and since
that password is a placeholder here, the server's own config guard refused to
start, so the harness failed for a reason unrelated to the artifact. Its
inputs are now RELEASE_* names that cannot collide with a deployment's.
* It shared a Compose project with the development database stack, both
deriving `pangolin` from the directory name. `down -v` in the release harness
deleted the dev stack's MinIO volume - which is exactly how the Postgres
compliance test started failing with NoSuchBucket midway through this work. A
maintainer verifying a release while a dev stack was up would have destroyed
their local volumes. It now declares `name: pangolin-release`, and its MinIO
no longer publishes host ports it never used.
Verified: 59 test targets, 370 tests, zero failures against live PostgreSQL,
MongoDB and MinIO. Release gate green end to end: all 10 checks, including an
authenticated tenant round trip and the assertion that the same write without
credentials is refused. clippy 31 at budget, fmt clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion `config drift` failed on the previous push: its "every compose file must be valid" step exports PANGOLIN_JWT_SECRET, but docker-compose.release.yml now takes RELEASE_JWT_SECRET and RELEASE_ROOT_PASSWORD - required via `:?`, so `docker compose config` cannot render the file without them. Renaming those inputs so a developer's .env could not feed the release harness broke the one job that renders every compose file. Also corrects a justification that my own fix made false. The drift guard exempts PANGOLIN_API_URL, and the comment explained that as "nothing reads it, because the script that service runs cannot run in that container". That was true when written and is not now: scripts/release_smoke_test.py reads it. The exemption is still needed - the guard greps the Rust server, and this is a Python script - but for a different reason, and a stale reason in a guard is how the guard stops meaning anything. All four steps of the job verified locally against the actual commands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements every finding in
roadmap_aug10.md— the full-repo audit of2026-08-10 — plus the improvements it recommended, and the follow-up work that
audit could not have found.
This is a security release. 0.6.0 carries a privilege escalation that any
authenticated principal can exploit. See
SECURITY.md for the
advisory and the upgrade guide
for the compatibility detail.
Verification
cargo test --workspacecargo clippycargo fmt --all --checkpytest/ruff(SDK)npm run build(UI)The security cluster
Ten issues exploitable with nothing but a valid credential — including the
lowest-privilege
tenant-useror any service-user API key:POST /api/v1/tokenstook no session at all and mapped abody-supplied
roles: ["Root"]into signed claims. Any authenticated callercould mint a
Roottoken for any tenant;check_permissionshort-circuitsfor
Root, so that token bypasses every subsequent check in the system.table, and hardcoded
read+write. Any tenant member obtained cloud storagecredentials for the entire warehouse.
session.user_id; revocation is keyed byjti, whichno token carries as its
user_id. Logout did nothing.client_credentialsfor afresh JWT, indefinitely.
PANGOLIN_DEV_MODEwaived theNO_AUTHpublic-bind guard — and thetwo flags are routinely set together.
rename_table,update_namespace_properties, the view endpointsand
perform_maintenancehad no authorization check at all.Plus B0i (cross-tenant grants), B0m (a remote panic in token issuance) and B0o
(unrevocable tokens).
What the guardrails found that review did not
Two suites were added because the entire authorization cluster was invisible to
CI — the code compiled, was formatted, was lint-clean, and the tests passed.
The permission matrix drives each sensitive route as Root / tenant admin /
ungranted tenant user / foreign tenant admin. Verified by reintroducing B0a
behind a one-line edit: it fails with
200 where 403 expected.The cross-backend parity suite runs the same assertions against all four
stores. On its first run it found:
SqliteStorehad no inherentrevoke_token, so the trait delegation calleditself — revoking a token on SQLite aborted the process.
audit_logstable declared different columns than the codeinserted, so that backend kept no audit trail at all.
Pointed at a live PostgreSQL and MongoDB for the first time, it found four more:
business_metadatawhilesearch_assetsjoins it.the deserializer expects Binary. Every role-derived permission silently
vanished: a user holding an admin role was authorized as though they held
none.
get_metadata_locationhad no fallback, so the read path and thecommit CAS disagreed about "current".
delete_catalogfailed outright on any standalonemongod.Also in here
default-spec-id,last-partition-id,"type": "struct",metadata-log; nested namespaces that registered underone key and were looked up under another (every commit to one 404'd); a
client able to jump the sequence counter to
i64::MAX; feature-branchcommits moving
main;create_tablesilently dropping every complex-typedcolumn.
revocation no-op, an orphaning branch delete, a search that panicked, a lost
CAS, and all three persistent backends rewriting 15 of 17 asset types to
IcebergTable.agreeing, so deployed builds called the visitor's localhost), ~13 raw
fetches that 404'd outside the dev proxy, three endpoints that did not exist,
no 401 handling, and a dead tenant switcher.
wrong fields, or nothing at all. All of it survived because every command
swallowed its error and exited 0.
docker compose upcould not start the API (B8), bothcompose files set a variable nothing reads (B9), and the release compose file
pinned an image four versions old running a script that does not exist (B10).
New CI
guardrails(authz matrix + parity against live Postgres/Mongo/MinIO, failingif a backend was skipped),
sdk,ui, andconfig-drift— which validatesthe compose files, the env-var reference, and that all five artifacts carry one
version.
Reviewing this
The commits are one per cluster and each message explains what was broken and
why it mattered. Reading them in order is the intended path.
Two things to weigh in on:
deny_unknown_fieldson request bodies, the branchsemantics change, and four SDK signatures. All are documented in the upgrade
guide with what each was silently doing before — but they are behaviour
changes and worth your judgement.
.envhas been removed and must be rotated. It wasnever tracked by Git but sat in plaintext and was passed into containers by
docker compose.🤖 Generated with Claude Code